Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Refactor] Unify repo search order by logic #30876

Merged
merged 11 commits into from
Jun 13, 2024

Conversation

6543
Copy link
Member

@6543 6543 commented May 6, 2024

have repo OrderBy definitions defined in one place and use a single type for OrderBy database options

@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 6, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 6, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label May 6, 2024
models/repo/search.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label May 6, 2024
@6543
Copy link
Member Author

6543 commented May 7, 2024

I would say this refactor is ready ;)

@6543 6543 requested a review from lunny May 7, 2024 12:16
@lunny
Copy link
Member

lunny commented May 7, 2024

I would say this refactor is ready ;)

I will review and merge after v1.22 stable is released since it's a refactor PR.

6543 added a commit that referenced this pull request May 7, 2024
similar to #30784 but only for the repo explore page

is covered by #30876 for the main branch
@6543 6543 added this to the 1.23.0 milestone May 7, 2024
@6543
Copy link
Member Author

6543 commented Jun 10, 2024

@lunny we now have v1.22.0 tagged :)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 12, 2024
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(LGTM although we should probably fix the uppercase problem before the merge)

routers/web/explore/repo.go Show resolved Hide resolved
models/repo/search.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 12, 2024
@6543
Copy link
Member Author

6543 commented Jun 12, 2024

@delvh that are two valid refactors :)

But as this is already taking that long i propose to make two small followup pulls adressing it :)

@delvh
Copy link
Member

delvh commented Jun 12, 2024

The first one is a bug, not a refactor.
And the bug is introduced through this PR:
You query the sorting using a potentially unsanitized value, so you need to sanitize (lowercase) it first beforehand

@6543 6543 enabled auto-merge (squash) June 13, 2024 08:55
@6543 6543 merged commit bb04311 into go-gitea:main Jun 13, 2024
26 checks passed
@6543 6543 deleted the unify-repo-search-order-by-logic branch June 13, 2024 10:15
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 14, 2024
* giteaofficial/main:
  Extract and display readme and comments for Composer packages (go-gitea#30927)
  Have new announcement about docs contributions (go-gitea#31364)
  Fix bug filtering issues which have no project (go-gitea#31337)
  Refactor to use UnsafeStringToBytes (go-gitea#31358)
  [skip ci] Updated translations via Crowdin
  Fix PullRequestList.GetIssueIDs's logic  (go-gitea#31352)
  [Refactor] Unify repo search order by logic (go-gitea#30876)
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Jun 18, 2024
go-gitea/gitea#30876 (comment)
(cherry picked from commit 78e8296e113e2fd9259ec05fe87035427821ea0b)
6543 added a commit that referenced this pull request Sep 1, 2024
Fix #31916

In #30876, `sortOrder` has been changed into a map, but it is only
implemented in explore.
~~But it seems that size sort order has no effect from long long ago,~~
not directly caused by the PR above.

I think it is still caused by #29231.
In #29231, it merged the sort orders from
`templates/explore/repo_search.tmpl` and
`templates/admin/repo/search.tmpl`.
In `templates/admin/repo/search.tmpl`, it contains size sort orders, but
not in `templates/explore/repo_search.tmpl`, which is used in non-admin
pages.
So `order by size` is added from #29231, but the handler was not added.

---------

Co-authored-by: 6543 <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants